src/rpp/rpp/operators/switch_on_next.hpp (1)

16-16: Remove unnecessary include directive

The header <array> is included but not used in this file. Unnecessary includes can increase compilation times and introduce unwanted dependencies.

Apply this diff to remove the unused include:

-#include <array>

Configuration used: CodeRabbit UI
Review profile: CHILL

Learnt from: victimsnino
PR: victimsnino/ReactivePlusPlus#673
File: src/rpp/rpp/operators/concat.hpp:59-59
Timestamp: 2024-11-10T04:25:44.618Z
Learning: In the `concat_disposable` class in `src/rpp/rpp/operators/concat.hpp`, within the `drain` method, it's acceptable to call `get_observer()->on_completed()` even when `is_disposed()` returns true, because `refcounted.dispose()` could cause disposing of `this` object.

#!/bin/bash
# Description: Search for all usages of 'on_completed' to verify consistent behavior.

# Expected: Instances where 'on_completed' is called with appropriate disposal checks.
rg 'on_completed\(' -A 3

#!/bin/bash
# Get the implementation of switch_on_next operator
ast-grep --pattern 'class switch_on_next_observer_strategy {
  $$$
}'

#!/bin/bash
# Get the content of switch_on_next.hpp to understand the completion logic
cat src/rpp/rpp/operators/switch_on_next.hpp

src/rpp/rpp/operators/switch_on_next.hpp (1)

25-25: Add override specifier to base_dispose_impl

The virtual method implementation should be explicitly marked with the override specifier for better code clarity and to catch potential signature mismatches at compile-time.

-        void base_dispose_impl(interface_disposable::Mode) noexcept override
+        void base_dispose_impl(interface_disposable::Mode) noexcept final override

Also applies to: 42-46

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Search for similar disposal patterns in other operators
rg -U "on_completed.*dispose.*is_disposed" --type cpp

-            auto new_inner = rpp::composite_disposable_wrapper::make();
-            {
-                auto inner = m_state->get_inner_child_disposable();
-                inner->dispose();
-                *inner = new_inner;
-            }
-            std::forward<T>(v).subscribe(switch_on_next_inner_observer_strategy<TObserver>{m_state, std::move(new_inner)});
+            auto new_inner = rpp::composite_disposable_wrapper::make();
+            {
+                auto inner_lock = m_state->get_inner_child_disposable().unique_lock();
+                inner_lock->dispose();
+                *inner_lock = new_inner;
+                std::forward<T>(v).subscribe(switch_on_next_inner_observer_strategy<TObserver>{m_state, std::move(new_inner)});
+            }

src/rpp/rpp/operators/switch_on_next.hpp (1)

Line range hint 25-52: LGTM! Consider adding documentation for the new members.

The refactoring from refcount_disposable to base_disposable with separate base and inner disposables improves the resource management design. The implementation correctly handles thread safety with mutex protection.

Consider adding documentation comments for the new member variables and methods to explain their roles:

/// Base disposable for managing the main subscription
rpp::composite_disposable m_base_child_disposable{};

/// Disposable for managing the current inner subscription, protected by mutex
rpp::utils::value_with_mutex<rpp::composite_disposable_wrapper> m_inner_child_disposable{};

src/tests/rpp/test_switch_on_next.cpp (1)

168-202: Good test coverage, but could be more descriptive and comprehensive

The test cases effectively verify completion behavior in various scenarios. However, consider these improvements:

1. Make test descriptions more descriptive:

-    SUBCASE("switch_on_next completes right")
+    SUBCASE("switch_on_next handles completion signals correctly")
-        SUBCASE("on_completed from base")
+        SUBCASE("completes when base observable completes with no active inner observable")
-        SUBCASE("on_completed from inner + then from base")
+        SUBCASE("completes when inner observable completes followed by base observable completion")
-        SUBCASE("on_completed from base + then from inner")
+        SUBCASE("completes when base completes followed by active inner observable completion")

2. Add comments explaining the expected behavior:

 SUBCASE("switch_on_next handles completion signals correctly")
 {
     mock_observer<int>    mock{};
     trompeloeil::sequence s{};

     rpp::subjects::publish_subject<rpp::dynamic_observable<int>> subj{};

     subj.get_observable() | rpp::ops::switch_on_next() | rpp::ops::subscribe(mock);
+    // Case 1: Base observable completes with no active inner observable
+    // Expected: Immediate completion
     SUBCASE("completes when base observable completes with no active inner observable")
     {
         REQUIRE_CALL(*mock, on_completed()).IN_SEQUENCE(s);
         subj.get_observer().on_completed();
     }

+    // Case 2: Inner observable completes, then base completes
+    // Expected: Completion only after base completes
     SUBCASE("completes when inner observable completes followed by base observable completion")
     {
         subj.get_observer().on_next(rpp::source::empty<int>());

         REQUIRE_CALL(*mock, on_completed()).IN_SEQUENCE(s);
         subj.get_observer().on_completed();
     }

+    // Case 3: Base completes, then active inner observable completes
+    // Expected: Completion only after inner observable completes
     SUBCASE("completes when base completes followed by active inner observable completion")
     {
         subj.get_observer().on_next(rpp::source::empty<int>());
         subj.get_observer().on_next(rpp::source::never<int>());

         rpp::subjects::publish_subject<int> inner{};
         subj.get_observer().on_next(inner.get_observable());
         subj.get_observer().on_completed();

         REQUIRE_CALL(*mock, on_completed()).IN_SEQUENCE(s);
         inner.get_observer().on_completed();
     }
 }

3. Consider adding these edge cases:
   • Multiple inner observables completing before base completion
   • Error scenarios during completion
   • Completion with buffered values

Would you like me to help implement the additional test cases?

Configuration used: CodeRabbit UI
Review profile: CHILL

-m_state->get_base_child_disposable().dispose();
-if (m_state->get_inner_child_disposable()->is_disposed())
+auto inner_disposed = m_state->get_inner_child_disposable()->is_disposed();
+m_state->get_base_child_disposable().dispose();
+if (inner_disposed)
    m_state->get_observer()->on_completed();

src/tests/rpp/test_switch_on_next.cpp (1)

144-175: LGTM: Thorough completion behavior testing

The new test case comprehensively verifies completion behavior in three key scenarios:

1. Base observable completion
2. Inner observable completion followed by base completion
3. Base completion followed by inner observable completion

Consider adding these additional scenarios to make the test coverage even more robust:

1. Multiple inner observables completing before base completion
2. Error scenarios during completion
3. Race conditions during completion

Example:

SUBCASE("multiple inner observables complete before base")
{
    auto inner1 = rpp::subjects::publish_subject<int>{};
    auto inner2 = rpp::subjects::publish_subject<int>{};
    
    subj.get_observer().on_next(inner1.get_observable());
    subj.get_observer().on_next(inner2.get_observable());
    
    inner1.get_observer().on_completed();
    inner2.get_observer().on_completed();
    
    REQUIRE_CALL(*mock, on_completed()).IN_SEQUENCE(s);
    subj.get_observer().on_completed();
}

Configuration used: CodeRabbit UI
Review profile: CHILL